Migrate the Site Creation Domain Search to wordpress-rs - #23247
Conversation
Generated by 🚫 Danger |
|
|
|
|
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #23247 +/- ##
=======================================
Coverage 37.81% 37.82%
=======================================
Files 2347 2347
Lines 128360 128362 +2
Branches 17770 17774 +4
=======================================
+ Hits 48542 48555 +13
+ Misses 75861 75851 -10
+ Partials 3957 3956 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Typing the first character hides the intro block and swaps the search icon for a spinner. `animateLayoutChanges` animated the resulting bounds change, which is what makes the screen jump and the keyboard flicker while the first results load. The views around the header now snap rather than animating. #23247 drops the attribute from the site creation search, where the same animation was taking focus off the search field.
|
There seem to be a few points it would be great if you could double-check:
|
Typing the first character hides the intro block and swaps the search icon for a spinner. `animateLayoutChanges` animated the resulting bounds change, which is what makes the screen jump and the keyboard flicker while the first results load. The views around the header now snap rather than animating. #23247 drops the attribute from the site creation search, where the same animation was taking focus off the search field.
* Migrate `DomainSuggestionsViewModel` from FluxC to wordpress-rs Fetch domain products and suggestions through `WpComApiClient` instead of `ProductsStore` and the `Dispatcher`/EventBus pair. Suspend calls remove the need for the `@Subscribe` handler and the dispatcher registration. Changes: - Swap `ProductsStore` and `Dispatcher` for `WpComApiClientProvider` and `AccountStore` - Map the `empty_results` rejection to an empty success, as FluxC's `SiteRestClient.suggestDomains` did - Carry the API's message into `ListState.Error`, and log failures with `toLogErrorString` - Retarget `Product.isOnSale` at the wordpress-rs `Product`, whose `saleCost` holds hundredths of the currency unit rather than a decimal amount - Extract `buildSuggestionsParams`, and cover the sale, error and superseded query paths * Cover the product type the suggestions screen requests Extract `buildProductsParams` so the filter can be asserted. `request` takes an opaque lambda, so a verify on the client counts calls without pinning which endpoint each one reached: swapping `ProductTypeFilter.Domains` for another filter left every test green while no suggestion would carry a sale price. Changes: - Extract `buildProductsParams` and assert it selects `Domains` - Rename `domain products are fetched only at first start` to name the repeat-start guarantee its call count actually covers * Bind each test observer to the list it was created with The observers closed over the `onDomainSelectedEvents` and `suggestionStates` fields, which `createViewModel` reassigns. A test that builds a second view model leaves the first one's observers writing into the second one's lists. * Name the non-API failure test after what it asserts The fallback to `R.string.domain_suggestions_fetch_error` happens in `DomainSuggestionsFragment`, which this test does not exercise. It asserts that `ListState.Error` carries no message. * Handle the suggestions response on the main thread `suggestions` is a plain field written from the search field, the debouncer and the response handler. FluxC pinned the handler to the main thread through `@Subscribe(threadMode = ThreadMode.MAIN)`; the coroutine that replaced it runs on `BG_THREAD`, so a response landing while `fetchSuggestions` is still running can be overwritten by the stale state that thread holds. `postValue` then coalesces the two, leaving the spinner up with no error reported. A request that fails without reaching the network, as it does with no connection, is fast enough to make the overlap likely. Changes: - Inject the `UI_THREAD` dispatcher and run `onDomainSuggestionsFetched` on it * Tell the user when a domain search failed for want of a connection The screen reported every failure as "Domain suggestions couldn't be loaded", including the one the user can act on. wordpress-rs distinguishes a request that never reached the network, so an offline search now says to check the connection. Changes: - Set `errorMessageResId` to `error_network_connection` when the result is a `DeviceIsOfflineError` - Prefer `errorMessageResId` over `errorMessage` in the fragment's toast * Drop the implicit layout animation from the domain search Typing the first character hides the intro block and swaps the search icon for a spinner. `animateLayoutChanges` animated the resulting bounds change, which is what makes the screen jump and the keyboard flicker while the first results load. The views around the header now snap rather than animating. #23247 drops the attribute from the site creation search, where the same animation was taking focus off the search field. * Report a failed domain search once A failed search with nothing already on screen showed two messages at once: the empty view said no domains were found while a toast said the suggestions could not be loaded. The empty view is attached to the list through `setEmptyView`, so it appears for any empty list including the one an error leaves behind. The failure now goes into the empty view when the list is empty, with the reason as its subtitle, and falls back to a toast only when there are still results on screen to keep. * Start the domain search at rest rather than loading The layout shipped the search icon `gone` and the progress bar visible, the inverse of the `tools:visibility` on each. The fragment corrects both the first time `suggestions` is assigned, so any path that reaches the screen without issuing a search leaves the spinner running with nothing behind it. * Clear the suggestions when a search fails The list answers the query in the search field, and `ListState.Error` is built from the previous state's data. Rejecting a query therefore left the previous query's results on screen beneath the new text, so searching `...` after a real search showed that search's domains as though they answered it. The results are already hidden while a request is in flight, so this stops a failure restoring them rather than introducing a new behaviour. With the list always empty on failure, the empty view carries every error and the toast goes away, which also stops the longer API messages being truncated. * Clear the search results when the field is emptied Emptying the field left the last result on screen, so an error explaining how a query has to be written stayed up after that query was deleted. Clearing now returns the list to `Init`, the state the screen opens in, and the fragment hides the list and its empty view there as it does while loading. The default search that follows an emptied field is unchanged, so the site's own suggestions still come back when they were not already showing. * Stop searching for a blank query A site with no name gives `initializeDefaultSuggestions` nothing to search for, on open and again whenever the field is emptied. The blank query went to the API, which rejects it as `invalid_query`, so emptying the field replaced the error it was meant to clear with an identical one. A blank query now leaves the list in `Init` without a request, which is the resting state the field's placeholder already implies. * Keep the suggestions through a configuration change Restoring the view state sets the search field's text, and `TextView` reports that as a change even when the text is empty, so a rotation tells the view model the query was cleared. Clearing reset the list to `Init`, and no fetch follows when the query is already the site name, so the retained suggestions were replaced by a blank screen with no way back but typing. Emptying the field now only resets the list when the query has moved off the site name, which is the case the reset exists for. * Clear the selected domain when no search is issued `fetchSuggestions` clears the selection at the end, and the blank-query guard returns before reaching it. Emptying the field on a site with no name left the Select button enabled over a list that had just been cleared, holding a domain the user could no longer see, and tapping it created a cart. The reset also moves above the request, so every write this thread makes to `suggestions` is done before a response can arrive on another one. The comment on the response handler claimed that as an effect of running on the main thread, which it never was — `fetchSuggestions` runs on the debouncer's scheduler, so the two threads overlapped either way. * Drop part of a comment in DomainSuggestionsViewModel that referred an earlier implementation * Report a missing WP.com account rather than assuming one `AccountStore.accessToken` is typed nullable but reads `""` when signed out, and is only null between an in-process sign out and the next launch. A null check therefore passed in the case it was written for and the screen sent `Authorization: Bearer ` for the server to reject, which arrived as the same generic failure as a dropped connection. In the case it did catch it threw inside `launch`, where nothing handles it. `getOrCreateClient` now returns null for either shape, matching `FetchAllDomainsUseCase`, and both requests report it instead: the product fetch logs and carries on, and the search shows the error the fragment already has a string for. * Write the suggestions state from one thread `suggestions` was read-modify-written from three threads: the debouncer's scheduler in `fetchSuggestions`, the main thread from the row taps and the response handler, and `BG_THREAD` from `showLoadingButton` during cart creation. Tapping Select as a response landed could interleave two of those cycles, dropping the fetched list or leaving rows enabled mid-purchase. `fetchSuggestions` and `showLoadingButton` now run on the main thread with the rest. The query is still read on the debouncer thread and passed in, so it remains the one the search was scheduled for rather than whatever the field holds by the time the main thread gets there.
32d2744 to
cc6f672
Compare
|
@adalpari Thank you for the review.
3-6. I'm confused. These items have nothing to do with this PR, do they? |
You are totally right. I used the full diff and ended up getting those points as well. Those ones are just UX suggestions anyway, so I think it's fine. |
adalpari
left a comment
There was a problem hiding this comment.
Thanks for checking my comment. LGTM!
… FluxC to wordpress-rs Replace FluxC's dispatcher/EventBus domain suggestions with direct wordpress-rs API calls: - `FetchDomainsUseCase` now uses `WpComApiClient.domains().suggestions()` instead of FluxC's `Dispatcher` + `OnSuggestedDomains` event - `SiteCreationDomainsViewModel` uses wordpress-rs for products fetch instead of `ProductsStore` - Handle `invalid_query` API error via `WpErrorCode.CustomException` to preserve the empty-results UX (matching FluxC's `INVALID_QUERY`) - Remove `Dispatcher` registration/unregistration from ViewModel
`quantity` is always `FETCH_DOMAINS_SIZE`, so the parameter was suppressed rather than read. No caller passed it. Changes: - Remove `size` from `FetchDomainsUseCase.fetchDomains` - Drop it from the stubs and verifications in `SiteCreationDomainsViewModelTest`
The caller passes the query in and holds it across the call, so it never read the copy coming back. Echoing it was needed when the result arrived over EventBus and had to be matched to its request. Changes: - Remove `query` from `FetchDomainsResult`, making `InvalidQuery` and `Error` data objects - Update the ViewModel branches and both test classes
Every rejection reported as a generic error with no message, and `empty_results` reached the user as a failure with a retry button rather than the empty-list message it stands for. `SuggestDomainErrorType` supplied the tracked type before, derived from the same API code; the tracker lowercases what it is given, so the code goes through as-is. Changes: - Carry the API error code and message on `FetchDomainsResult.Error` and track them instead of a hardcoded `GENERIC_ERROR` - Map `empty_results` to an empty `Success` - Log the product-fetch failure with `toLogErrorString()` - Cover the `invalid_query`, `empty_results`, and other-code paths, and assert the tracked type and message
Changes:
- Sort the `rs.wordpress` and `uniffi` imports after the `org.wordpress`
block in the ViewModel and its test, matching the other migrated classes
- Make `SALE_PRODUCTS_COUNT` a const and drop its `@Suppress("unused")`;
both sale tests read it
- Drop an `@Suppress("UNCHECKED_CAST")` from a test with no cast, and a
stray blank line
`SiteCreationDomainsViewModel` requested the products list on start and stored it in a field nothing read, so the screen paid for a request it made no use of. `NewDomainsSearchRepository` and `DomainSuggestionsViewModel` are the callers that read a product, both for `combinedSaleCostDisplay`, and neither is affected. Changes: - Remove the products fetch, the cached field, and the API client it needed, along with the `WpComApiClientProvider` and `AccountStore` dependencies - Remove the test asserting the fetch happened once, and the two `@Ignore`d sale tests it fed
`Cost.OnSale` and `Tag.Sale` were only ever built by `DomainItemPreview`, so the sale price and its tag rendered in Android Studio and nowhere else. Changes: - Remove `Cost.OnSale`, `Tag.Sale`, the `SalePrice` composable, and the branch selecting it - Remove them from the preview fixture - Remove the `site_creation_domain_cost_sale` and `site_creation_domain_tag_sale` strings they used
Typing the first character made the clear button and the spinner appear, and the `LayoutTransition` animated the neighbouring views' bounds through zero height. The framework clears focus on a view animated to zero size, so the field lost it, the toolbar's navigation button took it, and the input connection stayed inactive until the results arrived and the field asked for focus back. Every character typed in between was dropped. Both layouts belong to this screen alone. The header still animates itself in `updateHeader`; what is lost is the implicit animation of the views around it. Changes: - Drop `android:animateLayoutChanges` from `site_creation_domains_screen` and `site_creation_search_input_item`
Checkstyle rejects an empty line before a closing brace, and removing `Tag.Sale` and `Cost.OnSale` left one behind in each. Changes: - Drop the blank lines before the `Tag` and `Cost` closing braces
Lint's `ExtraTranslation` rejects a string that a locale translates but the default locale does not define, and the two sale strings left the default locale with the UI that used them. Changes: - Delete `site_creation_domain_tag_sale` and `site_creation_domain_cost_sale` from every `values-*/strings.xml`
`AccountStore.accessToken` is typed nullable but reads `""` when signed out, and is only null between an in-process sign out and the next launch. A null check therefore passed in the case it was written for and the search sent `Authorization: Bearer ` for the server to reject, which arrived as the same generic failure as a dropped connection. In the case it did catch it threw inside `launch`, where nothing handles it. `getOrCreateClient` now returns null for either shape, matching `FetchAllDomainsUseCase`, and the search reports it as its own error type so it stays separable from a request that reached the server. Changes: - Return null from `getOrCreateClient` for a null or empty token - Report `NO_ACCESS_TOKEN` from `fetchDomains`, and log it - Cover both token shapes
cc6f672 to
784fb8d
Compare


Description
Moves
SiteCreationDomainsViewModelandFetchDomainsUseCaseoff FluxC and onto wordpress-rs.FetchDomainsUseCasecallsWpComApiClient.domains().suggestions()directly instead of dispatchingnewSuggestDomainsActionand waiting for anOnSuggestedDomainsevent, so the EventBus subscription, the query-to-continuation pairing, and the ViewModel's dispatcher registration all go away.Behaviour is meant to match trunk. The pieces worth checking:
empty_resultsis not a failure. The API reports "no domains for that search" as an HTTP error, and FluxC turned it back into a successful empty list. The migration does the same, so the screen shows the empty-list message rather than a red error with a retry button.invalid_querykeeps its own message. Same as trunk, and it is not tracked as an error.FetchDomainsResult.Errorcarries the API's error code and message through totrackErrorShown.SuggestDomainErrorTypesupplied that value before, derived from the same code, and the tracker lowercases whatever it is given, so the value reported for a known code is unchanged.Three cleanups came out of reviewing the migration:
fetchDomainshad asizeparameter that was suppressed rather than read, sincequantityis alwaysFETCH_DOMAINS_SIZE. No caller passed it.FetchDomainsResultechoed thequeryback, which the caller already held. That existed to match an EventBus result to its request and a suspend call needs no such thing.NewDomainsSearchRepositoryandDomainSuggestionsViewModelare the callers that read a product, both forcombinedSaleCostDisplay, and neither is affected. The sale UI this screen kept for it —Cost.OnSale,Tag.Sale, theSalePricecomposable — was only ever built byDomainItemPreview, so it rendered in Android Studio and nowhere else.Dropping
animateLayoutChangeson this screenOn trunk, typing the first character into the domain search stops you typing until that first request finishes — every keystroke in between is lost. The implicit layout animation that runs when the clear button and the spinner appear takes focus off the search field, and nothing gives it back until the results arrive.
Removing
animateLayoutChangesfrom this screen's two layouts fixes it. The trade is that the views around the header snap into place rather than sliding. It reads as snappier to me and losing keystrokes was the worse of the two, but it is a visible change.Testing instructions
Reach the screen: My Site → the site name dropdown → + → Create WordPress.com site → skip or fill the two steps before Choose a domain.
Typing is not interrupted:
A query the API refuses:
.A query with no matches:
zcharacters.A normal search:
coolsite..wordpress.comoption among them.No connection:
Screenshots